Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

refactor: move emoji formatting into concrete fns #391

Merged
merged 2 commits into from
Aug 5, 2019

Conversation

ashleymichal
Copy link
Contributor

@ashleymichal ashleymichal commented Aug 3, 2019

doing this for some more flexibility in new message function formats. merging into master but this is relevant to KV commands work #392

@ashleymichal ashleymichal force-pushed the alewis/refactor-messages branch 2 times, most recently from c35cc5a to 5bed5f6 Compare August 3, 2019 22:26
@EverlastingBugstopper
Copy link
Contributor

This is great! Do you eventually want to deprecate external usage of message altogether? Perhaps we should open an issue for updating instances of message elsewhere in the codebase to use these new functions.

@ashleymichal
Copy link
Contributor Author

@EverlastingBugstopper there shouldn't be any external use of the message fn, as it is not public. everywhere you see message outside of this file it is used as a namespace. The purpose of this module is to centralize and standardize how we present information to the user in the terminal. the purpose of the refactor is to just move more of the formatting concerns into the module functions themselves rather than assume that every type of message is surrounded by a specific emoji.

Copy link
Contributor

@ashleygwilliams ashleygwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! i do think we should take @EverlastingBugstopper 's suggestion and file an issue to remove instances of message.

@ashleygwilliams ashleygwilliams merged commit f3d492a into master Aug 5, 2019
@ashleygwilliams ashleygwilliams deleted the alewis/refactor-messages branch August 5, 2019 14:46
@ashleygwilliams ashleygwilliams added this to the 1.1.1 milestone Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants